-
Notifications
You must be signed in to change notification settings - Fork 137
Evaluation tracing #1663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Evaluation tracing #1663
Conversation
|
Formatting check succeeded! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1663 +/- ##
============================================
+ Coverage 65.82% 65.88% +0.06%
Complexity 1645 1645
============================================
Files 474 477 +3
Lines 27454 27496 +42
Branches 5463 5467 +4
============================================
+ Hits 18071 18117 +46
+ Misses 7085 7083 -2
+ Partials 2298 2296 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
| cqlEngine: CqlEngine, | ||
| expression: String, | ||
| initialContext: Pair<String, Any>?, | ||
| initialContext: Pair<String, Any?>?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: This looks incorrect to me. What are the cases where you might have a context specified and set as null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on whether we want to allow something like "Unfiltered" to null for the context-value pair. Doing so is equivalent to not providing the pair at all in which case the State.contextValues map won't have the "Unfiltered" key and the lookup will return null anyway.
| context.getCurrentLibrary()?.identifier, | ||
| expression, | ||
| ) | ||
| if (exception.backtrace == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: What stops us from building the exception with the correct backtrace attached from the get-go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is to be attached always at the very beginning, we'll need to make the engine state or State.stack (the activation frame stack) available everywhere a CqlException or its subclass (InvalidCast, TerminologyProviderException, DataProviderException, etc.) is instantiated. TerminologyProviderException and DataProviderException are part of public provider APIs so they aren't concerned with the engine state. An alternative would be to extend or create a new CqlException when it enters the engine scope and that's what it effectively does currently (and all in one place).
| """ | ||
| .trimIndent(), | ||
| backtrace.toString(), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Nice cleanup / simplification!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!



TraceFrameclass in both evaluation traces and backtraces.Pair<String, Any?>.